Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of IRemoteVariables messages via RPC #2935

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

PeterBowman
Copy link
Member

Bugfix: a wrong pointer check could prevent from parsing remote IRemoteVariables commands.

@PeterBowman PeterBowman temporarily deployed to code-analysis January 23, 2023 17:32 — with GitHub Actions Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis January 23, 2023 17:32 — with GitHub Actions Inactive
@PeterBowman PeterBowman temporarily deployed to code-analysis January 23, 2023 17:32 — with GitHub Actions Inactive
@pattacini
Copy link
Member

cc @davidetome

@randaz81
Copy link
Member

Just beware, device ControlBoardWrapper is deprecated in yarp master (3.8).
I'm going to apply the same fix on a new commit to the new device controlBoard_nws_yarp (In yarp master the two devices do not share the same source code).

@randaz81 randaz81 merged commit 4178507 into robotology:yarp-3.7 Jan 31, 2023
randaz81 added a commit that referenced this pull request Jan 31, 2023
@PeterBowman PeterBowman deleted the cbw_remote_vars branch January 31, 2023 10:02
@PeterBowman
Copy link
Member Author

PeterBowman commented Jan 31, 2023

I'm going to apply the same fix on a new commit to the new device controlBoard_nws_yarp (In yarp master the two devices do not share the same source code).

Thank you, @randaz81. I was about to ask: the now deprecated NWS's implementation had been nicely split into multiple files using virtual inheritance (ref), but this change was lost in the new device (ref). Is this intentional? I could provide a PR (in the future, targetting 3.8.x) to restore that if needed.

If I may also ask: when is YARP 3.8.0 due? I'm hesitant to propose a discussion about reverting #2841 in 3.7.

Also, I noticed this in current master: 76fe34d#r97492632.

@randaz81
Copy link
Member

randaz81 commented Jan 31, 2023

Thank you, @randaz81. I was about to ask: the now deprecated NWS's implementation had been nicely split into multiple files using virtual inheritance (ref), but this change was lost in the new device (ref). Is this intentional? I could provide a PR (in the future, targetting 3.8.x) to restore that if needed.

Hi @PeterBowman, the files you are mentioning are no longer required, The logic of the old controlboardwrapper has been split between the nws (only network communication) and another device, the controlBoardremapper (joints map handling). So the implementation of that code you are mentioning has been absorbed by the remapper.

If I may also ask: when is YARP 3.8.0 due?

We were thinking at the end of January, we are already late. We'll do our best to merge the safest and easier PR in this release.

I'm hesitant to propose a discussion about reverting #2841 in 3.7.

This is a known issue to me and I know it's important. Let's discuss it (on a separated thread) and let's see if we can do something for this release or postpone it to Yarp 3.8.1 (eventually a couple of weeks later)

Also, I noticed this in current master: 76fe34d#r97492632.

Thank you! I'll fix it.

@PeterBowman
Copy link
Member Author

PeterBowman commented Jan 31, 2023

The logic of the old controlboardwrapper has been split between the nws (only network communication) and another device, the controlBoardremapper (joints map handling). So the implementation of that code you are mentioning has been absorbed by the remapper.

Oops, my bad, you are totally right, @randaz81. It looks like ControlBoardWrapper.{h/cpp} here are no longer used, then.

Let's discuss it (on a separated thread) and let's see if we can do something for this release or postpone it to Yarp 3.8.1

I will start an issue to cover that topic, but it is not my intention to delay the next release; it is fine for me if the solution arrives at some later point. For now, though, would you mind considering #2869 for 3.7.x (and also 3.8 by merging the patch into the new device)?

Edit: I have opened #2939 to discuss about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants